common.h: Make IS_ALIGNED() safe for testing with alignment == 0#10322
common.h: Make IS_ALIGNED() safe for testing with alignment == 0#10322jsarha wants to merge 1 commit intothesofproject:mainfrom
Conversation
Make IS_ALIGNED() safe for testing with alignment == 0. Without this fix the DSP will crash if alignemnet is 0. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a division-by-zero crash in the IS_ALIGNED() macro that occurs when checking alignment with a value of 0. The issue manifests when mod_balloc() calls mod_balloc_align() with align=0, which then triggers an assertion using IS_ALIGNED() that would perform a modulo operation by zero.
Key Changes:
- Modified
IS_ALIGNED()macro to handle zero alignment values safely by short-circuiting when alignment is 0
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| /* Align the number to the nearest alignment value */ | |||
There was a problem hiding this comment.
The macro lacks documentation explaining the behavior when alignment is 0. Add a comment clarifying that zero alignment is treated as always aligned (returns true), which prevents division by zero in the modulo operation.
| /* Align the number to the nearest alignment value */ | |
| /* Align the number to the nearest alignment value */ | |
| /* If alignment is zero, IS_ALIGNED returns true (treats all values as aligned). | |
| * This prevents division by zero in the modulo operation. | |
| */ |
| /* Align the number to the nearest alignment value */ | ||
| #ifndef IS_ALIGNED | ||
| #define IS_ALIGNED(size, alignment) ((size) % (alignment) == 0) | ||
| #define IS_ALIGNED(size, alignment) (!(alignment) || (size) % (alignment) == 0) |
There was a problem hiding this comment.
I think we are using Zephyr version (included above) and this version of IS_ALIGNED wont be used (#ifndef IS_ALIGNED)
#ifdef __ZEPHYR__
#include <zephyr/sys/util.h>
#endif
btw alignment 0 is incorrect anyway so maybe this macro was called somewhere where it shouldnt?
There was a problem hiding this comment.
Whether it is incorrect or not, is a matter of definition. But apparently this change is useless on its own, so I'll close it.
|
The Zephyr definition is the one that is used in SOF, not this. So this is an idle attempt to fix the issue. |
2 participants
Make IS_ALIGNED() safe for testing with alignment == 0. Without this fix the DSP will crash if alignemnet is 0.
This is why:
The current virtual heap implementation has this line:
assert(IS_ALIGNED(mem, align));
And mod_balloc() implementation looks like this:
static inline void *mod_balloc(struct processing_module *mod, size_t size)
{
return mod_balloc_align(mod, size, 0);
}
Which means, that without this fix any call to mod_balloc() would cause division by zero crash.
I think the correct place to fix this error is in IS_ALIGNED() macro. It should not crash if someone wants to check alignment to zero.